Skip to content

Fix order notification deeplink #8042

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Nov 4, 2022
Merged

Fix order notification deeplink #8042

merged 2 commits into from
Nov 4, 2022
+3 −4

Conversation

@ealeksandrov
Copy link
Contributor

@ealeksandrov ealeksandrov commented Nov 4, 2022

Fixes #8020

Description

This PR updates updates push notification deeplink condition. Previously it only continued after the store switch (for multi-store notifications).

How

  • Added @MainActor to prevent hitting thread assertion. It doesn't impact release build, but is a blocker for debug.
  • Removed unnecessary condition from presentNotificationDetails func.

Testing

  1. Test on the device.
  2. Agree to push notifications on the first app run or update it in the iOS settings for the Woo app.
  3. Test foreground notification
    1. Run the app and keep it in foreground.
    2. Make a new order from the desktop to trigger a notification.
    3. Tap "view" on in-app banner.
    4. Confirm it switches to the orders tab and opens order details.
    5. Navigate back to dashboard.
  4. Test background notification
    1. Run the app and dismiss it, navigating to the home screen (don't force close it).
    2. Make a new order from the desktop to trigger a notification.
    3. Tap iOS notification for the order.
    4. Confirm app launches, switches to the orders tab and opens order details.
    5. Navigate back to dashboard.
  5. Test notification for the closed app
    1. Force close the Woo app (swipe up from app switcher).
    2. Make a new order from the desktop to trigger a notification.
    3. Tap iOS notification for the order.
    4. Confirm app launches, switches to the orders tab and opens order details.
    5. Navigate back to dashboard.

Note: sometimes in "closed app" state the flow doesn't navigate fully to the order details, stopping on top of refreshed orders list. I wasn't able to find the issue or reliably reproduce it.

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@ealeksandrov ealeksandrov added feature: notifications Related to notifications or notifs. priority: high Affects lots of customers substantially, but not critically. labels Nov 4, 2022
@ealeksandrov ealeksandrov added this to the 11.1 milestone Nov 4, 2022
@ealeksandrov ealeksandrov changed the title Issue/8020 fix push deeplink Fix order notification deeplink Nov 4, 2022
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Nov 4, 2022

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr8042-aeb02b5 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@oguzkocer oguzkocer modified the milestones: 11.1, 11.2 Nov 4, 2022
@Ecarrion Ecarrion self-assigned this Nov 4, 2022
@oguzkocer oguzkocer changed the base branch from trunk to release/11.1 November 4, 2022 20:12
@Ecarrion Ecarrion removed their assignment Nov 4, 2022
Ecarrion
Ecarrion approved these changes Nov 4, 2022
View reviewed changes
Copy link
Contributor

@Ecarrion Ecarrion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested all 3 scenarios and all worked well! 🎉

The only scenario that I couldn't test was when another site was selected because I don't seem to be getting multi-store push notifications

@Ecarrion Ecarrion merged commit 4b78b10 into release/11.1 Nov 4, 2022
@Ecarrion Ecarrion deleted the issue/8020-fix-push-deeplink branch November 4, 2022 20:39
@shiki
Copy link
Contributor

shiki commented Nov 4, 2022 •
edited
Loading

I tested this as well. For the first few tries, it didn't work. I wonder if it was because of the lock screen.

After successfully testing the foreground notification scenario, though, everything else worked fine. ¯\_(ツ)_/¯

@Ecarrion
Copy link
Contributor

Ecarrion commented Nov 4, 2022

For the first few tries, it didn't work. I wonder if it was because of the lock screen.

Weird, my first test was with the app totally killed 🤔

@oguzkocer oguzkocer mentioned this pull request Nov 5, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rachelmcr rachelmcr Awaiting requested review from rachelmcr

1 more reviewer

@Ecarrion Ecarrion Ecarrion approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

feature: notifications Related to notifications or notifs. priority: high Affects lots of customers substantially, but not critically.

Projects

None yet

Milestone

11.2

Development

Successfully merging this pull request may close these issues.

Clickin on order push notifications doesn't open order details

6 participants

@ealeksandrov @wpmobilebot @shiki @Ecarrion @oguzkocer